Skip to content

Minor CMake change to better integrate c++17 and modernize GTest-integration#174

Merged
JohanMabille merged 2 commits intoxtensor-stack:masterfrom
BioDataAnalysis:bda_gtest_upgrade
Oct 27, 2020
Merged

Minor CMake change to better integrate c++17 and modernize GTest-integration#174
JohanMabille merged 2 commits intoxtensor-stack:masterfrom
BioDataAnalysis:bda_gtest_upgrade

Conversation

@emmenlau
Copy link
Contributor

@emmenlau emmenlau commented Oct 21, 2020

This is a minor PR that improves the integration of c++17 and of testing, and specifically of GTest. It brings better compatibility with FindGTest from (future) cmake.

Please let me know if it would need more motivation? :-)

@emmenlau
Copy link
Contributor Author

emmenlau commented Oct 21, 2020

The AppVeyor build error seems unrelated, unless I'm mistaken? It says:

LINK: command "C:\PROGRA~2\MI0E91~1.0\VC\bin\amd64\link.exe /nologo @CMakeFiles\test_xtensor_blas.dir\objects1.rsp /out:test_xtensor_blas.exe /implib:test_xtensor_blas.lib /pdb:C:\projects\xtensor-blas\test\test_xtensor_blas.pdb /version:0.0 /MANIFEST:NO /debug /INCREMENTAL /subsystem:console C:\xtensor-conda\Library\mingw-w64\lib\libopenblas.dll.a C:\xtensor-conda\Library\lib\gtest_main-mdd.lib C:\xtensor-conda\Library\lib\gtest-mdd.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib" failed (exit code 1120) with the following output:
gtest-mdd.lib(gtest-all.cc.obj) : warning LNK4099: PDB 'gtestd.pdb' was not found with 'gtest-mdd.lib(gtest-all.cc.obj)' or at 'C:\projects\xtensor-blas\test\gtestd.pdb'; linking object as if no debug info
gtest-mdd.lib(gtest-all.cc.obj) : error LNK2019: unresolved external symbol __std_reverse_trivially_swappable_4 referenced in function "void __cdecl std::_Reverse_unchecked1<enum testing::internal::edit_distance::EditType *>(enum testing::internal::edit_distance::EditType * const,enum testing::internal::edit_distance::EditType * const,struct std::integral_constant<unsigned __int64,4>)" (??$_Reverse_unchecked1@PEAW4EditType@edit_distance@internal@testing@@@std@@YAXQEAW4EditType@edit_distance@internal@testing@@0U?$integral_constant@_K$03@0@@Z)
test_xtensor_blas.exe : fatal error LNK1120: 1 unresolved externals
NMAKE : fatal error U1077: 'C:\xtensor-conda\Library\bin\cmake.exe' : return code '0xffffffff'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\amd64\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\amd64\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\BIN\amd64\nmake.exe"' : return code '0x2'

@emmenlau
Copy link
Contributor Author

emmenlau commented Oct 21, 2020

Ah but the build error on Azure is related. I think my change only works with cmake >= 3.18. I'll investigate...

@emmenlau emmenlau force-pushed the bda_gtest_upgrade branch 2 times, most recently from e82a80e to 22d822a Compare October 21, 2020 13:27
@emmenlau
Copy link
Contributor Author

emmenlau commented Oct 21, 2020

Ok, it seems my changes trigger a very rare problem with Visual Studio, that symbol __std_reverse_trivially_swappable_4 is not resolved. Its no well known and I could not find a clear solution, but reports indicate that a linking is performed with a Visual Studio linker that is older than the one that produced the Google test binaries.

I'll try to build with a downloaded GTest to confirm this.

@emmenlau emmenlau force-pushed the bda_gtest_upgrade branch 2 times, most recently from 5c6dcaa to 7a14938 Compare October 21, 2020 16:38
@JohanMabille
Copy link
Member

JohanMabille commented Oct 22, 2020 •
edited
Loading

Ok, it seems my changes trigger a very rare problem with Visual Studio, that symbol __std_reverse_trivially_swappable_4 is not resolved. Its no well known and I could not find a clear solution, but reports indicate that a linking is performed with a Visual Studio linker that is older than the one that produced the Google test binaries.

Thanks for catching this, it looks like this is the reason for the failure of #172.

JohanMabille
JohanMabille reviewed Oct 22, 2020
View reviewed changes
@JohanMabille
Copy link
Member

JohanMabille commented Oct 22, 2020

Looks like there are two unrelated changes here, improving gtest support and changes relative to C++17 support. Although these changes are small, they have enough impact on the codebase so that it makes sense to split the PR.

@emmenlau
Copy link
Contributor Author

emmenlau commented Oct 22, 2020

Yes you are right - I'll split and update the comments.

@JohanMabille JohanMabille merged commit 8afdee5 into xtensor-stack:master Oct 27, 2020
@emmenlau emmenlau deleted the bda_gtest_upgrade branch October 27, 2020 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohanMabille JohanMabille JohanMabille left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@emmenlau @JohanMabille